Add Exchange before GroupId to improve Partial Aggregation by aaneja · Pull Request #24047 · prestodb/presto

aaneja · 2024-11-14T12:07:34Z

See #23475 for more details

Previously closed PR - #11741

Description

Motivation and Context

See Javadoc of the new AddExchangesBelowPartialAggregationOverGroupIdRuleSet

Impact

Better performance for TPCDS Q22, Q67
See plan diffs (TPCDS SF 1000, unpartitioned) - https://aaneja.github.io/mypages/PR_24047_AddExchangesBelowPartialAggregationOverGroupId_OffVsOn.html

Test Plan

TODO : Add a new planner test

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Added a new optimizer rule to add exchanges below a combination of partial aggregation+ GroupId . Enabled with the boolean session property `enable_forced_exchange_below_group_id`

steveburnett · 2024-11-14T14:30:20Z

Thanks for the release note entry! Minor formatting nits, and include the PR number.

== RELEASE NOTES ==

General Changes
* Add a new optimizer rule to add exchanges below a combination of partial aggregation+ GroupId. Enabled with the boolean session property ``enable_forced_exchange_below_group_id``. :pr:`24047`

ZacBlanco

high level first pass. Seems good for the most part. I will take another pass and look at the details of the rule tomorrow.

rschlussel · 2025-01-22T19:27:09Z

maybe this should have 3 possible values - ALWAYS, COST_BASED, and NEVER (similar to partial aggregation pushdown). that way someone can enable this if they don't have stats or if the stats estimates are no good.

What would we re-partition on if ALWAYS is chosen (for the non-trivial case of more than one partition variable) ?

good point. i guess we can leave as is for now, unless we want to has on all of them?

I'm in favor of leaving this as-is until a use case arises for ALWAYS

rschlussel · 2025-01-28T17:37:49Z

what about a withProjection test. Also a test that it doesn't fire if it's disabled, only has one grouping set, has pass through keys.

'only has one grouping set' , added with this test

withProjection, does not fire if disabled -> will add

only has one grouping set, has pass through keys -> I could not build a use-case where this occurs. My understanding of when this could occur is unclear. Can you help me out with an example ?

sorry, i didn't mean one grouping set with pass through keys, i meant the case covered by https://github.com/prestodb/presto/pull/24047/files#diff-2e788a27c31ea3e4d5d404dba18eee33a3868d656bae3b0c0380269afb838f24, so multiple grouping sets that all share some common keys.

@rschlussel I think I've covered all the test cases now. Please take a look

steveburnett · 2025-02-03T15:05:37Z

Thanks for the release note entry! Minor formatting nits, and include the PR number.

== RELEASE NOTES ==

General Changes
* Add a new optimizer rule to add exchanges below a combination of partial aggregation+ GroupId. Enabled with the boolean session property ``enable_forced_exchange_below_group_id``.

The minor formatting nits should still apply, but new release note guidelines as of last week: PR #24354 automatically adds links to this PR to the release notes. Please remove the manual PR link in the following format from the release note entries for this PR.

:pr:`12345`

I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link.

aaneja · 2025-02-04T04:56:01Z

@steveburnett Thanks for the feedback! We're still working through some naming+defaults for the new session/feature flags. I will update the release notes + optimizer docs once we close on this

ZacBlanco

lgtm. one minor thing

aaneja · 2025-02-07T15:17:30Z

I noticed that we only create a GroupId node iff the size of the grouping sets is >1. So adding this invariant here, made sense. No tests failed, so IMO this should be added cc: @rschlussel

Based on: trinodb/trino@dc1d66fb co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com> Based on : trinodb/trino@c573b34 co-authored-by: Lukasz Stec <lukasz.stec@starburstdata.com> Based on: trinodb/trino@29328d3 co-authored-by: praveenkrishna <praveenkrishna@tutanota.com>

prestodb-ci added the from:IBM PR from IBM label Nov 14, 2024

aaneja force-pushed the groupIdExchangeOptimization branch from 95fb49c to 62aaab6 Compare November 14, 2024 12:13

aaneja force-pushed the groupIdExchangeOptimization branch from 62aaab6 to fa61dfd Compare November 18, 2024 12:58

aaneja force-pushed the groupIdExchangeOptimization branch from fa61dfd to 39222b3 Compare January 9, 2025 17:03

aaneja force-pushed the groupIdExchangeOptimization branch 2 times, most recently from 86b145a to 5868a2f Compare January 21, 2025 14:34

aaneja marked this pull request as ready for review January 21, 2025 14:35

aaneja requested review from a team, elharo, feilong-liu, jaystarshot and vivek-bharathan as code owners January 21, 2025 14:35

aaneja requested review from presto-oss and rschlussel January 21, 2025 14:35

ZacBlanco reviewed Jan 22, 2025

View reviewed changes

aaneja added a commit to aaneja/mypages that referenced this pull request Jan 24, 2025

Plan diff for prestodb/presto#24047

605bfd6

aaneja force-pushed the groupIdExchangeOptimization branch from 5868a2f to 437a09a Compare January 24, 2025 05:16

rschlussel reviewed Jan 28, 2025

View reviewed changes

aaneja force-pushed the groupIdExchangeOptimization branch from 437a09a to 979d204 Compare February 3, 2025 12:47

aaneja force-pushed the groupIdExchangeOptimization branch 2 times, most recently from 0874d82 to f95fab9 Compare February 4, 2025 06:13

ZacBlanco previously approved these changes Feb 4, 2025

View reviewed changes

Comment thread ...presto/sql/planner/iterative/rule/AddExchangesBelowPartialAggregationOverGroupIdRuleSet.java Outdated

aaneja dismissed ZacBlanco’s stale review via dcf0800 February 6, 2025 16:22

aaneja force-pushed the groupIdExchangeOptimization branch 4 times, most recently from e196f21 to 74178b4 Compare February 7, 2025 13:21

aaneja commented Feb 7, 2025

View reviewed changes

aaneja force-pushed the groupIdExchangeOptimization branch from 74178b4 to 84cafec Compare February 7, 2025 15:18

rschlussel approved these changes Feb 11, 2025

View reviewed changes

ZacBlanco approved these changes Feb 11, 2025

View reviewed changes

aaneja merged commit e84e4d2 into prestodb:master Feb 11, 2025

ZacBlanco mentioned this pull request Feb 11, 2025

Fix compilation failure after merging AddExchangeBeforeGroupId #24535

Merged

This was referenced Mar 10, 2025

Add release notes for 0.292 unix280/presto#5

Closed

Add release notes for 0.292 unix280/presto#6

Closed

prestodb-ci mentioned this pull request Mar 28, 2025

Add release notes for 0.292 #24825

Merged

30 tasks

unidevel mentioned this pull request Apr 25, 2025

Add release notes for 0.292 unix280/presto#23

Closed

30 tasks

This was referenced May 6, 2025

Add release notes for 0.292 unix280/presto#28

Closed

Add release notes for 0.292 unix280/presto#29

Closed

agrawalreetika mentioned this pull request Sep 16, 2025

[Doc] Correct session property details in release notes #26046

Merged

6 tasks

Conversation

aaneja commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett commented Nov 14, 2024

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveburnett commented Feb 3, 2025

Uh oh!

aaneja commented Feb 4, 2025

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aaneja commented Nov 14, 2024 •

edited

Loading